Prevent SelfDrop context mutation across render boundaries#2082
Prevent SelfDrop context mutation across render boundaries#2082karreiro wants to merge 5 commits into
SelfDrop context mutation across render boundaries#2082Conversation
aswamy
left a comment
There was a problem hiding this comment.
Not sure if liquid-spec also allows render tags, but ill update it to capture your test. Thank you for this ❤️
…drop_context_test.rb
ianks
left a comment
There was a problem hiding this comment.
We likely want to undef context as well, which serves as a signal that the drop doesn’t support contextualization
|
Thank you for the review, @ianks! I did consider |
74943a1 to
bc7f1ca
Compare
|
Hi 👋 This looks like a subtle change in policy for the I understand if you've decided this is a positive new feature. It's just that with this change partial templates are less isolated. They are potentially more tightly coupled to their parent templates, which I thought was one of the reasons for introducing And, of course, having arbitrary aliases of |
|
Thank you for the review, @jg-rp! That's a fair concern indeed. I had the same instinct when I first saw this, but talking with @ianks clarified this in my mind:
The positive feature we're introducing with The isolation boundary is still there. It just has a handle to a drop that was deliberately handed to it. I hope this clarifies the concern and motivation. Thanks again for the review! |
Co-authored-by: Ian Ker-Seymer <ian.kerseymer@shopify.com>
|
Thank you for the review, @ianks! I think you meant That said, I trust your instinct here. But I want to flag the complexity this introduces. Because case arg
when Liquid::Drop
drop.context = c if drop.respond_to?(:context=)
...Most call sites on Liquid already had this, but If the goal is signaling that a drop is not contextualizable, a method that communicates intent directly might be cleaner: # drop.rb
def contextualizable?
true
endThen Happy to keep the |
This PR fixes
SelfDropscoping so aselfassigned in one template preserves its original scope when passed to a rendered template.# template1.liquid {% assign var = 42 %} {% assign s = self %} {% render 'template2', other_self: s %} # template2.liquid {% assign var = 43 %} {{ other_self.var }} => 43 (wrong — should be 42)The fix renames
@contextto@self_contextinSelfDrop, so the drop has a specific reference to its originating context instead of reusing the inheritedcontextthat gets overwritten at thecontext.rblevel on line 220.